fix: localize decimal separator - #2889
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the number-cell renderer in the Tables Vue frontend to format decimal values using the user’s current locale (e.g., comma as decimal separator in de_DE), addressing Issue #1570’s incorrect separator display.
Changes:
- Switch number display formatting from
toFixed()to locale-awareIntl.NumberFormatusing the Nextcloud locale. - Add Cypress component coverage to verify locale-specific decimal separators and fixed decimal precision.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/shared/components/ncTable/partials/TableCellNumber.vue | Formats displayed numeric values using a locale-aware formatter instead of a hardcoded . decimal separator. |
| cypress/component/TableCellNumber.cy.js | Adds component tests validating localized decimal separator output and preserved decimal precision. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| const fractionDigits = Number(this.column?.numberDecimals ?? 0) | ||
| return new Intl.NumberFormat(getCanonicalLocale(), { | ||
| maximumFractionDigits: fractionDigits, | ||
| minimumFractionDigits: fractionDigits, | ||
| useGrouping: false, | ||
| }).format(this.value) |
There was a problem hiding this comment.
Good catch — clamped numberDecimals to the 0..20 integer range Intl.NumberFormat accepts (via Math.trunc + min/max) so out-of-range values no longer throw RangeError when rendering. Added a Cypress case for an oversized decimal count.
There was a problem hiding this comment.
did you actually verify that 0 to 100 fraction digits don't work?
There was a problem hiding this comment.
You were right to question that. I re-checked it in the browser runtime instead of relying on the earlier Node result: Chromium 151 accepts minimumFractionDigits/maximumFractionDigits values from 0 through 100 and rejects 101 with RangeError. Node 18 rejects values above 20, but this code runs in the browser, so the browser behavior is the relevant one here.
Pushed b59aa28 to change the clamp from 20 to 100, and updated the Cypress component test so numberDecimals: 101 renders exactly 100 fraction digits.
There was a problem hiding this comment.
Please attach screenshot or video of this working then. Seems from this thread there's confusion about what's valid and what's not
Fixes nextcloud#1570 Clamp fraction digits to the 0..20 range accepted by Intl.NumberFormat so out-of-range column settings cannot throw RangeError while rendering. Signed-off-by: xhon-pelushi <xhon@pelushi.com>
6d33a0c to
aec7e1f
Compare
There was a problem hiding this comment.
do we need to modify NumberForm.vue too?
There was a problem hiding this comment.
Yes. I added min="0" max="100" step="1" to the column NumberForm.vue decimals input in b59aa28, so normal edits stay inside the same range.
I also checked the row value editor and found the same invalid persisted-data risk there: it used raw numberDecimals for both String.repeat() and toFixed(). The same commit now normalizes decimals to 0..100 before both getStep and parseValue, with Cypress coverage for numberDecimals: 101 on the row form path.
Signed-off-by: xhon-pelushi <xhon@pelushi.com>
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Use locale-aware decimal separators.
Fixes #1570
Test plan